[Unit Test improvements] Use getMockBuilder rather than getMock directly #12990

photodude · 2016-11-23T16:49:20Z

Pull Request for Issue getMock was used directly.

Summary of Changes

Use getMockBuilder rather than getMock directly
This is needed to avoid situations that would cause PHP Fatal error: Call to protected method PHPUnit_Framework_TestCase::getMock() on newer versions of phpunit

Also fixes a missing method in the application cms mock needed for phpunit 5.6 compatibility, and fixes a mockdatabase issue in the JInstallerAdapter tests on hhvm

Testing Instructions

Merge by code review, Travis tests pass

Documentation Changes Required

none

Avoid situations that would cause `PHP Fatal error: Call to protected method PHPUnit_Framework_TestCase::getMock()` on newer versions of phpunit

mbabker · 2016-11-23T16:55:16Z

I was about to say I thought some of these were changed in the 4.0 branch, but looking it up it looks like what was changed actually dealt with some of the classes becoming abstract versus concrete (so using the getMockForAbstractClass() method.

Looks fine to me.

photodude · 2016-11-23T17:20:00Z

@mbabker The next big thing to fix in the unit tests after this is merged are the Assert Tags

PHPUnit_Framework_Assert::assertTag is deprecated: 59x
    10x in JFormFieldRadioTest::testGetInput
    6x in JFormTest::testGetInput
    3x in JFormFieldCheckboxesTest::testGetInputValueArrayNoChecked
    3x in JFormFieldCheckboxesTest::testGetInputValueNoChecked
    3x in JFormFieldCheckboxesTest::testGetInputNoValueNoChecked
    3x in JFormFieldCheckboxesTest::testGetInputNoValueOneChecked
    3x in JFormFieldCheckboxesTest::testGetInputValuesNoChecked
    3x in JFormFieldCheckboxesTest::testGetInputValueChecked
    3x in JFormFieldCheckboxesTest::testGetInputNoValueTwoChecked
    2x in JFormFieldTest::testSetup
    2x in JFormFieldTest::testGetLabel
    2x in JHtmlBatchTest::testItem
    2x in JHtmlBatchTest::testUser
    2x in JHtmlBatchTest::testAccess
    2x in JHtmlListTest::testUsers
    2x in JHtmlBatchTest::testLanguage
    1x in JFormFieldHiddenTest::testGetInput
    1x in JHtmlBootstrapTest::testRenderModal
    1x in JHtmlBootstrapTest::testStartAccordion
    1x in JHtmlBootstrapTest::testAddTab
    1x in JFormTest::testGetLabel
    1x in JHtmlBootstrapTest::testStartTabSet
    1x in JHtmlBootstrapTest::testaddSlide
    1x in JHtmlListTest::testPositions

photodude · 2016-11-23T18:03:05Z

Ugh, it appears there are nearly 2000 more places where we need to Use getMockBuilder rather than getMock directly

mbabker · 2016-11-23T18:06:26Z

Do it on the 4.0 branch and that should be cut in half easily 😉

Fixes 10 hhvm failures that have one of the following messages ```php UnexpectedValueException: No columns found for #__extensions table Expectation failed for method name is equal to <string:parseSchemaUpdates> when invoked 1 time(s). Method was expected to be called 1 times, actually called 0 times. ```

photodude · 2016-11-26T18:12:55Z

@mbabker I decided that I might as well just fix the test with hhvm failures in this PR.

rdeutz · 2016-11-28T19:28:16Z

hope to find time to test it, thanks for the work on this

photodude · 2016-12-04T22:52:46Z

@rdeutz you are welcome. I hope we can get this merged soon.

photodude · 2016-12-09T00:40:46Z

Once this PR is merged we can also close issue #10220

photodude · 2016-12-09T16:12:50Z

After this PR is merged we can consider moving HHVM out of the allowed failures on Travis.

csthomas · 2016-12-09T16:19:36Z

tests/unit/suites/libraries/joomla/feed/JFeedParserTest.php

@@ -184,7 +183,7 @@ public function testProcessElementWithEntry()
 	public function testFetchNamespace()
 	{
 		// Set a mock namespace into the namespaces for the parser object.
-		$mock = $this->getMock('JFeedParserNamespace');
+		$mock = $this->getMockBuilder('JFeedParserNamespace')->getMock();;


Code style: ;;

Thanks for catching that, I have fixed this.

csthomas · 2016-12-09T16:20:11Z

tests/unit/suites/libraries/joomla/document/feed/renderer/JDocumentRendererAtomTest.php

-			)
-		);
+		JFactory::$application = $this->getMockBuilder('JApplication')
+								->setMethods(array('get', 'getCfg', 'getRouter',))


Code style ',)'

I'm not sure what your referencing here arrays should end with a comma

For one line array I suggest to not use comma before ')'
array('get', 'getCfg', 'getRouter' =>,<= )

Whey they're multiline arrays you should end it with a comma. A single line array like this one, the trailing comma can be left off.

For single line array, can or should leave off the trailing comma? I generally take the stance of always including the trailing comma.

I don't think it's set in stone anywhere, but the general convention across the board (not just here in Joomla land) is single line arrays don't have a trailing comma while multiline arrays do.

If it helps speed getting this approved and merged.... I have made the changes

csthomas · 2016-12-09T16:20:26Z

tests/unit/suites/libraries/joomla/document/feed/renderer/JDocumentRendererRSSTest.php

-			)
-		);
+		JFactory::$application = $this->getMockBuilder('JApplication')
+								->setMethods(array('get', 'getCfg', 'getRouter',))


Code style ',)'

csthomas · 2016-12-09T17:04:42Z

tests/unit/suites/libraries/cms/installer/JInstallerAdapterTest.php

-		$mockTableExtension = $this->getMock('JTableExtension', array('find', 'load'), array($this->getMockDatabase()));
+		$mockTableExtension = $this->getMockBuilder('JTableExtension')
+							->setMethods(array('find', 'load'))
+							->setConstructorArgs(array(self::$driver))


Which way is correct? Use self:driver or $this->getMockDatabase() as below?

There is some issue with this specific test using $this->getMockDatabase() on HHVM that causes 9 failures and 1 error. Using self:driver in this one test resolves all of those issues.

This probably is not related to current PR.

If I correctly understand $this->getMockDatabase() simulates a connection to the database.
Then using self::$driver is correct.

It should be used too in method below testCheckExistingExtensionForExtensionThatDoesNotExist.

Notice: Parent class TestCaseDatabase always create a real connection to sqlite in self::$driver. Probably we do not need to use a mock database at all.

I agree "probably is not related to current PR"

photodude · 2016-12-10T20:54:23Z

@csthomas With the requested changes implemented, would you mark your code review successful?

csthomas · 2016-12-10T21:02:17Z

Yes. I wanted to check success it yesterday but I have tried to understand the way how does $this->getMockDatabase() work on php versions but not for hhvm. There are other tests with mock database and passed which I do not understand for now.

This is not directly related to this PR.

csthomas · 2016-12-10T21:03:03Z

I have tested this item ✅ successfully on b5d6a6d

Code review.

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/12990.}

rdeutz · 2016-12-11T11:43:16Z

@photodude Thanks, this was a really big PR and a lot of work. Could you make smaller parts in the future, please, then there is not so much danger that we run into conflicts :-)

photodude · 2016-12-11T17:06:50Z

@rdeutz, you are welcome, thank you for merging. I do generally try to avoid such large PRs. Like the code style PRs I've done, I've keep those to single components. I agree the large PRs are difficult to review and cause other issues like merge conflicts across other open PRs; and that should generally be avoided.

photodude added 16 commits November 23, 2016 08:43

Use getMockBuilder rather than getMock directly

4f99ff5

Avoid situations that would cause `PHP Fatal error: Call to protected method PHPUnit_Framework_TestCase::getMock()` on newer versions of phpunit

Use getMockBuilder rather than getMock directly

c4ca3b9

Use getMockBuilder rather than getMock directly

9771819

Use getMockBuilder rather than getMock directly

e6262cd

Use getMockBuilder rather than getMock directly

a3bb8de

Use getMockBuilder rather than getMock directly

56df791

Use getMockBuilder rather than getMock directly

e149f28

Use getMockBuilder rather than getMock directly

fd692ca

Use getMockBuilder rather than getMock directly

db25133

Use getMockBuilder rather than getMock directly

cd90aee

Use getMockBuilder rather than getMock directly

7e6c2cd

Use getMockBuilder rather than getMock directly

3e0fd3c

Use getMockBuilder rather than getMock directly

e3fee85

Use getMockBuilder rather than getMock directly

44b5de8

Use getMockBuilder rather than getMock directly

733bd85

Use getMockBuilder rather than getMock directly

adaa7f3

joomla-cms-bot added PR-staging Unit/System Tests labels Nov 23, 2016

Use getMockBuilder rather than getMock directly

4612c4a

joomla-cms-bot removed the Unit/System Tests label Nov 23, 2016

Use getMockBuilder rather than getMock directly

5c658b3

joomla-cms-bot added the Unit/System Tests label Nov 23, 2016

Use getMockBuilder rather than getMock directly

5e3c911

joomla-cms-bot removed the Unit/System Tests label Nov 23, 2016

Use the correct $this object

6e6dfd6

joomla-cms-bot added the Unit/System Tests label Nov 23, 2016

joomla-cms-bot added the Unit/System Tests label Nov 26, 2016

photodude mentioned this pull request Nov 26, 2016

HHVM errors from Travis ci #10220

Closed

rdeutz self-assigned this Nov 28, 2016

Merge branch 'staging' into patch-14

55fffa2

photodude mentioned this pull request Dec 1, 2016

CS: convert CRLF to LF #12381

Closed

Merge branch 'staging' into patch-14

1f16272

rdeutz added this to the Joomla 3.7.0 milestone Dec 5, 2016

photodude added 2 commits December 6, 2016 17:03

Merge branch 'staging' into patch-14

ead659e

Merge branch 'staging' into patch-14

7b64d3e

Merge branch 'staging' into patch-14

7b69531

csthomas reviewed Dec 9, 2016

View reviewed changes

photodude added 3 commits December 9, 2016 09:37

cs-fix extra semicolon

0e9fd59

remove trailing coma

ad8af69

remove trailing comma

144c6a9

csthomas reviewed Dec 9, 2016

View reviewed changes

rdeutz merged commit d9e1f90 into joomla:staging Dec 11, 2016

photodude deleted the patch-14 branch December 11, 2016 16:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Unit Test improvements] Use getMockBuilder rather than getMock directly #12990

[Unit Test improvements] Use getMockBuilder rather than getMock directly #12990

photodude commented Nov 23, 2016 •

edited

Loading

mbabker commented Nov 23, 2016

photodude commented Nov 23, 2016 •

edited

Loading

photodude commented Nov 23, 2016

mbabker commented Nov 23, 2016

photodude commented Nov 26, 2016

rdeutz commented Nov 28, 2016

photodude commented Dec 4, 2016

photodude commented Dec 9, 2016

photodude commented Dec 9, 2016

csthomas Dec 9, 2016

photodude Dec 9, 2016

csthomas Dec 9, 2016

photodude Dec 9, 2016

csthomas Dec 9, 2016 •

edited

Loading

mbabker Dec 9, 2016

photodude Dec 9, 2016

mbabker Dec 9, 2016

photodude Dec 9, 2016

csthomas Dec 9, 2016

csthomas Dec 9, 2016 •

edited

Loading

photodude Dec 9, 2016

csthomas Dec 9, 2016 •

edited

Loading

photodude Dec 9, 2016

photodude commented Dec 10, 2016

csthomas commented Dec 10, 2016

csthomas commented Dec 10, 2016

rdeutz commented Dec 11, 2016

photodude commented Dec 11, 2016

[Unit Test improvements] Use getMockBuilder rather than getMock directly #12990

[Unit Test improvements] Use getMockBuilder rather than getMock directly #12990

Conversation

photodude commented Nov 23, 2016 • edited Loading

Summary of Changes

Testing Instructions

Documentation Changes Required

mbabker commented Nov 23, 2016

photodude commented Nov 23, 2016 • edited Loading

photodude commented Nov 23, 2016

mbabker commented Nov 23, 2016

photodude commented Nov 26, 2016

rdeutz commented Nov 28, 2016

photodude commented Dec 4, 2016

photodude commented Dec 9, 2016

photodude commented Dec 9, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

csthomas Dec 9, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

csthomas Dec 9, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

csthomas Dec 9, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

photodude commented Dec 10, 2016

csthomas commented Dec 10, 2016

csthomas commented Dec 10, 2016

rdeutz commented Dec 11, 2016

photodude commented Dec 11, 2016

photodude commented Nov 23, 2016 •

edited

Loading

photodude commented Nov 23, 2016 •

edited

Loading

csthomas Dec 9, 2016 •

edited

Loading

csthomas Dec 9, 2016 •

edited

Loading

csthomas Dec 9, 2016 •

edited

Loading